@roji What do you think about this?

Thanks for the ping.

Yeah, I don't we should be doing this - not in this PR, in any case. We don't currently flow "for query" vs "for update" into the query pipeline for any other case; our current approach to this is instead to rewrite the main target table from the view to the table, specifically when translating ExecuteUpdate (see code). The advantage of that approach is keeping the view vs. table concern out of the query pipeline, and dealing with it only where it's relevant.

So the existing code (linked to above) should take care of everything (including for complex properties) - in fact I'm surprised it does not do so already. What exactly prompted you to introduce this change?

I'm also noting that we should only be referencing the table for the mutation; any property accesses elsewhere (e.g. in the WHERE clause) should reference the view.

If we do want to change the way we're dealing with this and filter the information into the query pipeline, we should probably do this more generally in a separate PR, and remove the current code switching from the view to the table which I linked to above.

please check this comment for initial thoughts about the issue
#34627 (comment)

before the change, we didnt have view metadata for complex types

the test mentioned above broke cuz of this change
it was relying on complex props not having mapping in the view ( container expression ) , it throws key not found , as expected in the test

after the change
now we have metadata for complex types, execute update find a mapping in the view
try to execute something like this
update viewx set column = ..

which throws exception cuz viewx isnot a table and can't be updated

so to keep the old test behavior and not break it, i had to know if i need table mappings only or viewortablemappings

select expression mapping of complex property does use getviewortablemapping
which return = view ?? table
so you need to distinguish if you need select expression for update / read statements accordingly

I'm also noting that we should only be referencing the table for the mutation; any property accesses elsewhere (e.g. in the WHERE clause) should reference the view.

not sure about this part.
for example i want to update table where a column is x

there is a where condition but because we are in an update statement we cant use the view at all

so mutation uses table regardless of where/set

after the change
now we have metadata for complex types, execute update find a mapping in the view
try to execute something like this
update viewx set column = ..

So as I wrote above, we already have code in the ExecuteUpdate translation which is supposed to take care of this - I'm wondering what it doesn't work.

Note that I'm not necessarily opposed to the approach here: flowing forUpdate in like this PR proposes is in some ways probably better than the current approach of doing a translation, and then rewriting that translation to transform the view to the table (always better do just do the right thing immediately rather than do one thing and rewrite later, and this would certainly simplify the ExecuteUpdate logic by removing the transformation...).

But if we do go in this direction, we should switch to it fully, rather than doing one thing for complex properties and another for regular entities.

@AndriySvyryd any thoughts?

@roji
sorry for the ping again :)
I had time last weekend to work on this again
I figured out what was the issue with the mentioned piece of code you linked above
I drafted a change for it in a new branch (reverted the commit where i added forUpdate flag first)
that fixed the old test: (issue link: #34706)
s => s.SetProperty(f => f.ComplexThing, new Context34677.ComplexThing { Prop1 = 3, Prop2 = 4 })

also added tests for new case which also passes now:
s => s.SetProperty(f => f.ComplexThing.Prop1, 6)

i opened a new pr for it , not sure if that's correct , should i close this one?
PR: #37072

also should i update the failing test baseline (Microsoft.EntityFrameworkCore.Scaffolding.CompiledModelSqlServerTest.ComplexTypes) ? is this part of the ci/cd or i should update it locally and push as part of the pr ?

Note that I'm not necessarily opposed to the approach here: flowing forUpdate in like this PR proposes is in some ways probably better than the current approach of doing a translation, and then rewriting that translation to transform the view to the table (always better do just do the right thing immediately rather than do one thing and rewrite later, and this would certainly simplify the ExecuteUpdate logic by removing the transformation...).

I agree that using the correct Metadata from the start is better than rewriting it later, but this change should be done in a separate PR.

also should i update the failing test baseline (Microsoft.EntityFrameworkCore.Scaffolding.CompiledModelSqlServerTest.ComplexTypes) ? is this part of the ci/cd or i should update it locally and push as part of the pr ?

Updating the baseline in the PR is expected

Note that I'm not necessarily opposed to the approach here: flowing forUpdate in like this PR proposes is in some ways probably better than the current approach of doing a translation, and then rewriting that translation to transform the view to the table (always better do just do the right thing immediately rather than do one thing and rewrite later, and this would certainly simplify the ExecuteUpdate logic by removing the transformation...).

I agree that using the correct Metadata from the start is better than rewriting it later, but this change should be done in a separate PR.

the new pr have the fixes to existing logic without doing the hacky 'forUpdate boolean'
all tests passes now, updated the baseline for the failing test.

• existing tests for the mentioned issue is also fixed (works instead of expecting an exception), added one more case for setting a property on the complex type.

Should I close this pr as all commits here needed for the new pr in order to fix the two issues #36894 , #34706 ?